Haven't tested the demo for this PR just yet as I'm a bit tied up with TCEI changes, but this is my first-pass review just taking a look at the code. Will test the demo tomorrow!

Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @IshavSohal)

src/components/image-editor.vue line 327 at r1 (raw file):

this.configFileStructure.assets['shared'].file(this.imagePreviews[idx].id) ? 'shared' : this.lang

This is used both here and in the const fileSource line assignment above, it may be worth assigning it to its own variable at the top of the code block to prevent code duplication.

src/components/image-editor.vue line 351 at r1 (raw file):

            console.log('imagePreviews');
            console.log(JSON.parse(JSON.stringify(this.imagePreviews)));
            for (let i = 0; i < this.panel.items.length; i++) {

If we don't need to use the item index for anything, I'm thinking we can just use the following to save a couple of lines:

for(asset in this.panel.items) {
  const assetSrc = asset.src.split('/');
  ...
}

src/components/image-editor.vue line 358 at r1 (raw file):

                const assetFolder = assetSrc[2];
                const assetId = assetSrc[3];
                if (this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared') {

You can just use the following here:

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared';

src/components/image-editor.vue line 372 at r1 (raw file):

            const assetFolder = assetSrc[2];
            const assetId = assetSrc[3];
            if (this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared') {

Same as above, you can just use this here:

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared'

src/components/image-editor.vue line 425 at r1 (raw file):

this.configFileStructure.assets['shared'].file(imageFile.id) ? 'shared' : this.lang

Same as before, since this is used multiple times (here and in the else block below), it may be worth assigning it earlier to reduce code duplication.

src/components/metadata-editor.vue line 879 at r1 (raw file):

        // If uploadLogo is set, upload the logo to the directory.
        // Q: if we create a shared assets folder, should the logo just go there, since both langs

Someone can shut me down if I'm wrong here, but I don't know if we should necessarily assume that the logo will be the same for both products. The best example I can think of would be if the product logo has some sort of text in it and the author wants to have an English and a French version of it.

I'm not sure how common of a case that would be though. It may be worth a discussion at some point.

src/components/video-editor.vue line 333 at r1 (raw file):

this.configFileStructure.assets['shared'].file(this.videoPreview.id) ? 'shared' : this.lang

Same as the image file, this should probably be stored assigned to a variable earlier to prevent code duplication.

src/components/video-editor.vue line 377 at r1 (raw file):

            // An assets src value is "out of date" if the asset exists in the shared asset folder, but still
            // references the curr lang's assets folder
            if (this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared') {

return this.configFileStructure.assets['shared'].file(assetId) && assetFolder !== 'shared'

src/components/video-editor.vue line 407 at r1 (raw file):

this.configFileStructure.assets['shared'].file(this.videoPreview.id) ? 'shared' : this.lang

Same as the previous comments about this!

When deleting an asset and its source count hits 0, it gets removed from configFileStructure. However the asset doesn't get removed from the assets folder of the product. Due to this, when the app is reloaded and configFileStructure is initialized, the deleted asset returns. If this is a bug, I can open a new issue for this.

This seems like a bug to me. We don't want stale assets floating around in storylines products. Someone please correct me if I'm wrong and missing a use case for this.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)

src/components/image-editor.vue line 225 at r1 (raw file):

            oppositeSourceCount = this.sourceCounts[oppositeFileSource] ?? 0;
            this.sourceCounts[oppositeFileSource] = 0;
            this.configFileStructure.assets[oppositeLang].remove(file.name);

I think we need to perform this on this.lang's folder. While following the PR test steps, I noticed that the img remains in the original langs asset folder after it has been moved to the shared folder.

Scenario "Adding an asset to configs of both langs" indicates that the image should be removed from the assets/en folder, however, it persists.

src/components/metadata-editor.vue line 879 at r1 (raw file):

Agreed and I think that will be a relatively common case.

When deleting an asset and its source count hits 0, it gets removed from configFileStructure. However the asset doesn't get removed from the assets folder of the product. Due to this, when the app is reloaded and configFileStructure is initialized, the deleted asset returns. If this is a bug, I can open a new issue for this.

If I'm understanding this correctly, it's probably this same issue we've had for a while. See the following: #343, #111

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @IshavSohal)

src/components/image-editor.vue line 221 at r1 (raw file):

        // in the shared nor the opposite langs assets folder, the file should be uploaded to the current
        // langs assets folder
        if (this.configFileStructure.assets[oppositeLang].file(file.name)) {

I've found a bug that most likely relates to this section. Try out these steps:

1. Create a new image panel using the English configuration.
2. Upload an image.
3. Save the product.
4. Swap to the French configuration and create a new image panel.
5. Upload the same image you uploaded to the English slide.
6. Save the product.

The image is correctly moved to the shared folder, however, the image source for the panel in the English configuration still references the en folder when it should be referencing the shared folder.

This block will need additional code that updates the source path in the opposite language config with the new shared path. It looks like the video editor is missing this too, so we'll need there too.

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @IshavSohal)

src/components/image-editor.vue line 351 at r1 (raw file):

In hindsight, I don't think it actually works as I've written it here. You can ignore it, or this.panel.items.forEach(asset => { ... }); should work.

Yeah that's exactly what I was referring to

Reviewable status: 2 of 7 files reviewed, 11 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

src/components/image-editor.vue line 221 at r1 (raw file):

I ended up creating a shared-asset event, which is emitted whenever an asset is to be moved to the shared assets folder. The handler for this event will look for each instance of the uploaded asset within the opposite lang's config, and will update its source to refer to the shared asset folder. Is this the approach you had in mind? Let me know if you still have this bug.

src/components/image-editor.vue line 225 at r1 (raw file):

I think this may be related to the bug I mentioned (the assets not being deleted from the product in the server). Whenever the product is re-fetched from the server, configFileStructure gets updated such that it contains any/all deleted assets.

If the product is not re-fetched from the server, this issue shouldn't occur, but correct me if I'm wrong.

src/components/image-editor.vue line 327 at r1 (raw file):

Donethanks, I created a method srcFolder() to do this

src/components/image-editor.vue line 351 at r1 (raw file):

Due to the new shared-asset event this method isn't needed anymore, so I just deleted it.

src/components/image-editor.vue line 358 at r1 (raw file):

Deleted this method, all good now

src/components/image-editor.vue line 372 at r1 (raw file):

Deleted this method, all good now

src/components/image-editor.vue line 425 at r1 (raw file):

Donethanks

src/components/metadata-editor.vue line 879 at r1 (raw file):

Ah fair point. However upon creating a product, both configs now have a src for their respective logos that refers to the assets/en folder.

src/components/video-editor.vue line 333 at r1 (raw file):

Donethanks

src/components/video-editor.vue line 377 at r1 (raw file):

Deleted this method, all good now

src/components/video-editor.vue line 407 at r1 (raw file):

Donethanks

Reviewed 2 of 6 files at r1, 4 of 5 files at r2, all commit messages.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @IshavSohal and @szczz)

src/components/editor.vue line 218 at r2 (raw file):

Is this the approach you had in mind?

I think this approach should work 👍

src/components/editor.vue line 229 at r2 (raw file):

        console.log(JSON.parse(JSON.stringify(oppositeConfig.slides)));

        for (let i = 0; i < oppositeConfig.slides.length; i++) {

This (and the panel loop just below this) can be simplified with something like the following:

oppositeConfig.slides.forEach(oppositeSlide => { ... });

The variable oppositeSlide can be changed to whatever you think works!

src/components/editor.vue line 239 at r2 (raw file):

                console.log(JSON.parse(JSON.stringify(currPanel)));

                // if panel is slideshow, image or video then proceed

This will miss the dynamic panel, which like slideshows can have any slide type as a child (including slideshows).

It may be nice to use a recursive solution here like we use in other parts of the app in order to reduce code duplication (see an example here, although we can simplify it).

The recursive function would look something like the following (disclaimer: wrote this directly in Reviewable and haven't tested it so it may have some syntax errors or bugs):

const recursiveHelper = (panel: BasePanel) => {
  switch (panel.type) {
    case 'slideshow':
      panel.items.forEach(item => recursiveHelper(item));
      break;
    case 'dynamic':
      panel.children.forEach(child => recursiveHelper(child));
      break;
    default:
      // your code to handle the individual panels here.
      // If we want to ensure this only runs on images and videos, we can add a check for that here too.
      break;
  }
}

After you have this, you should just need to call it like this:

currSlide.panel.forEach(panel => { recursiveHelper(panel); }

Reviewable status: 2 of 9 files reviewed, 5 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

a discussion (no related file):
I've made one additional change. With the new redesigns merged in, I noticed that the configLang prop was not updating appropriately when changing from the en config to the fr config (since they now both reside in the same editor view). I've created a new lang-change event that is emitted whenever a slide-change event is handled by the editor component. The lang-change event is handled in the metadata-editor component.

src/components/editor.vue line 229 at r2 (raw file):

Donethanks

src/components/editor.vue line 239 at r2 (raw file):

Donethanks, appreciate the help!

Reviewed 5 of 7 files at r3, all commit messages.
Reviewable status: 7 of 9 files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @szczz)

server/index.js line 12 at r3 (raw file):

const simpleGit = require('simple-git');
const responseMessages = [];
require('dotenv').config();

I think this file snuck into this PR somehow! Other than that, all of the tests I've done look good!

Reviewable status: 5 of 9 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA and @szczz)

server/index.js line 12 at r3 (raw file):

This was just me fixing some formatting issues I noticed. Everything should be working the same as before.

Reviewed 1 of 7 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @szczz)

server/index.js line 12 at r3 (raw file):

👍

Adding an asset to configs of both langs

After performing these steps, but with a slight modification of copying the config from the en slide. The assets still remain in the en folder. They get moved to the shared folder if you manually add the same image to the FR slide though.

Reviewed 1 of 7 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)

server/index.js line 12 at r3 (raw file):

Yep please remove this file from the PR

slight modification**

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

Donethanks. As I was making the fix for this, I decided to create a helper method, panelHelper(), which can be provided a callback (as well as additional arguments for it) to be called on each ImagePanel/VideoPanel within a given BasePanel. This may be a bit overkill, so let me know your thoughts/opinions.

Reviewable status: 4 of 10 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA and @szczz)

server/index.js line 12 at r3 (raw file):

Donethanks

Reviewed 4 of 6 files at r5, all commit messages.
Reviewable status: 8 of 10 files reviewed, 1 unresolved discussion (waiting on @IshavSohal and @szczz)

a discussion (no related file):

Found a bug:

1. Create a new blank slide.
2. Change both the English and French slide to an "image" slide.
3. Upload a photo to the English slide.
4. Upload the same photo to the French slide.
5. Switch back to the English slide and notice that the image doesn't appear. Switching over to the Advanced panel shows that the src still points to the en folder instead of shared.

This seems to work fine for slideshow panels, just breaks with image panels.

Note: loading in existing products with old configuration asset folder structure will freeze the metadata page and eventually resolve with all N/A values. This breaks backwards compatibility and should be considered before merging without a plan in place to migrate old products.

Reviewed 1 of 6 files at r1, 2 of 7 files at r3, 1 of 3 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @IshavSohal)

src/components/image-editor.vue line 130 at r1 (raw file):

    mounted(): void {
        console.log('');

Blocker for console logs

Reviewable status: 2 of 10 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

a discussion (no related file):

Donethanks, should be all good now

This may have been due to the console logs that I've now removed, which were problematic for large products.

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA, @szczz, and @yileifeng)

a discussion (no related file):
Removed all console logs.

Couple of issues:

1. If you create a new storyline, upload a logo to the EN config and then save it, the logo gets added to the en assets folder as expected. If you return to the dashboard and load the storyline and then change to the french config, the original logo remains. If you go to the editor page and then press save, it results in the logo being stored in both the en fold and fr folder even though its exactly the same.

2. If you create a new slide and upload an image with the name "test.jpeg" in the en config, then flip to the fr config and upload a different image with the name "test.jpeg", returning to the en config will display the second image. If you save the en config before the french one, the second image will show up in the shared folder, but the first image will remain in the en folder. Perhaps a hash function or looking at image metadata could help handle this situation.

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @yileifeng)